-
-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/colors update #12072
Feat/colors update #12072
Conversation
I've found some discrepancies between data provided and designs. Clarifying now with David which ones are correct. So I'll make it draft till resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it looks like it doesn't broke anything in Suite. But without warranty 🫣
@@ -37,18 +37,21 @@ const light = { | |||
|
|||
// Figma Colors | |||
backgroundAlertBlueBold: palette.lightAccentBlue600, | |||
backgroundAlertBlueBoldAlt: palette.lightAccentBlue700, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw what does the Alt mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say ALTERNATIVE, but dunno, it was in the export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! The "bold-alt" tokens were added as a hotfix to the current token taxonomy due to missing colors for hover states of alert buttons. They intentionally have this generic name because we didn't want to unnecessarily add more new tokens than necessary for expanding alert buttons to new variants (red, yellow, blue). We would have to create a completely separate group with tokens "default" "hover/pressed", or rename the current tokens from "bold" to "default", which, considering the interconnectedness of tokens in various components, didn't seem like the best idea and would add unnecessary complexity. Therefore, it is primarily a temporary solution for the currently set taxonomy and naming, which is largely outdated, and that's why we are working on a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells to me like temporarily–permanent solution 🫣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way.. work is already underway on the new semantics due to the shortcomings of the current one (issues with accessibility, scalability, and maintenance as our system gradually expands). Unfortunately, it's quite a broad topic that requires extensive analysis to harmonize the requirements for several touchpoints in both design and code simultaneously. Here, we need to address support for multiple platforms simultaneously, while also considering the design aspect and the need to implement a brand facelift with new colors throughout the entire system and so on..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, got it :)
I've received new export so the colors will change. |
source data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and it looks fine
/rebase |
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/8754222584 |
4208c93
to
32b0835
Compare
Updating colors based on export from figma
Prerequisity to #11931